Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.20%. Comparing base (1c0769f) to head (3b18374).

@@                     Coverage Diff                     @@
##           od/resetGlobalScheduler    #3136      +/-   ##
===========================================================
+ Coverage                    73.17%   73.20%   +0.02%     
===========================================================
  Files                          430      430              
  Lines                       104034   104154     +120     
  Branches                     16727    16738      +11     
===========================================================
+ Hits                         76129    76242     +113     
- Misses                       27629    27636       +7     
  Partials                       276      276              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@filikat this branch now uses HiGHS#latest to run the Julia tests, as well as the tests from jump-dev/MultiObjectiveAlgorithms.jl#211 (not that I can reproduce the segfault there though).

I wonder if it's something like this:

1. We start one HiGHS instance
2. We run, do a bunch of stuff
3. We start a new HiGHS instance
4. We run, do a bunch of stuff
5. Sometime during (4), the GC runs and collects the first HiGHS instance, including calling Highs_destroy.

This is totally possible. In the C api, Highs_destroy not only deletes the Highs instance, but also kills the parallel scheduler. The scheduler relies on static variables, so there is only one of it, regardless of how many Highs instances are created. If you create two Highs instances, destroy the first and still use the second, this may cause issues because the parallel scheduler is terminated while the second instance is still using it.

I don't think that this should happen. The destructor of the static instance of ExecutorHandle already kills the scheduler at process termination, so technically there is no need to shut it down explicitly. I wonder if calling shutdown explicitly is done to solve some of the issues that we were having on Windows. Maybe @galabovaa @jajhall have some insights into this.
Maybe we should have a Highs_finalise function in the C api that does not depend on any instance and simply kills the scheduler, so that it is not done by Highs_destroy.

Tomorrow I will check if any of the unit tests creates this exact situation of two separate instances where one is destroyed before the other. If not, I will write such a test and try to verify if the issue is there.
One easy way to check is also to comment the line Highs::resetGlobalScheduler(true); in Highs_destroy.

One easy way to check is also to comment the line Highs::resetGlobalScheduler(true); in Highs_destroy.

Ooooooo.

Based on this scary warning, it seems highly likely that's the cause:

I may be able to fix this in JuMP, because we explicitly mark that the GC can run during a call to Highs_run:

https://github.com/jump-dev/HiGHS.jl/blob/5f3e3b7a6af3b988186f64934416f0241d5eefb2/src/MOI_wrapper.jl#L2295-L2297

Added by #2493

There is already oid Highs_resetGlobalScheduler(const HighsInt blocking); in the C API.

The C API should never call resetGlobalScheduler because you can never do so in a thread-safe way. This must be called only by the user.

So I think this was probably the cause of the failures in #2998. I should have taken a deeper look, instead of just working around the cause.

I'm fairly happy that resetGlobalScheduler was the culprit. Once #3141 is merged I'll update this PR to be just the changes for the Julia CI tests.

The C API should never call resetGlobalScheduler because you can never do so in a thread-safe way. This must be called only by the user.

I agree. Indeed, the cpp functions that run Highs never call resetGlobalScheduler explicitly. It is always called by the user after Highs has finished running.

Am I right in thinking that nothing is lost by removing 'resetGlobalScheduler' from the C API method that destroys HiGHS.

All we need is a C API method that calls 'resetGlobalScheduler' so that we, and users, can achieve this from C. Something we'll need for CI tests?

There already is such a function, Highs_resetGlobalScheduler.
We need to make sure that users are aware that this should be called by them when they are finished using all Highs instances.

Am I right in thinking that nothing is lost by removing 'resetGlobalScheduler' from the C API method that destroys HiGHS.

I think so. It was added to avoid some valgrind errors when running the minimal C example, but the same can be achieved by calling Highs_resetGlobalScheduler explicitly in call_highs_from_c_minimal.c, like Oscar did.

Something we'll need for CI tests?

The tests in #3141 are passing, so I don't think there's much else to fix.

Let's merge #3141 into latest, and then I'll re-target this PR to merge into latest after that.